Make multi-arg options optional #334

Rishi-0007 · 2024-10-26T13:00:17Z

Summary:

This PR fixes issue #331, where single-value options in the CLI handler were incorrectly consuming positional arguments. The parser now correctly distinguishes between single-value and multi-value options, ensuring options like -k or --kit consume only one argument, while options like --plugins can accept multiple values.

Key Changes:

Updated Option Interface:
- Added acceptsMultipleValues?: boolean to indicate if an option accepts multiple values.
Modified option Method:
- Now accepts an acceptsMultipleValues parameter (default is false).
Adjusted parseOptions Method:
- Single-value options consume only the next argument.
- Multi-value options collect arguments until another option or -- is encountered.
- Introduced -- as a separator between options and positional arguments.
Updated processSubCommand Method:
- Utilizes the index from parseOptions to correctly handle positional arguments.
Added Tests:
- Verified that single-value options do not consume positional arguments.
- Ensured multi-value options handle multiple values correctly using -- separator.

Examples of Corrected Behavior:

Single-Value Option:
```
npx create-robo -k activity myactivity
```
- Parsed Options: { kit: 'activity' }
- Positional Arguments: ['myactivity']
Multi-Value Option with Separator:
```
npx create-robo --plugins @robojs/ai @robojs/sync -- myactivity
```
- Parsed Options: { plugins: ['@robojs/ai', '@robojs/sync'] }
- Positional Arguments: ['myactivity']

Impact:

Fixes Issue: Single-value options no longer consume positional arguments.
Maintainer's Feedback Addressed: Multi-value options are supported as intended.

Testing:

Tests Passing:
- All new and existing tests pass successfully.
Validation:
- Confirmed that options and positional arguments are parsed correctly.

Notes:

Users should use -- to separate multi-value options from positional arguments when necessary.
Documentation should be updated to reflect these changes and guide users accordingly.

Thank you for reviewing this PR.

…gle values Ensure options like capture only one argument, leaving positional arguments intact.

vercel · 2024-10-26T13:00:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
robo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 28, 2024 8:37pm

Pkmmte · 2024-10-26T13:00:31Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Rishi-0007 · 2024-10-26T13:06:11Z

I have read the CLA Document and I hereby sign the CLA

Pkmmte · 2024-10-26T21:46:55Z

Thank you for this! Can you please verify that multi-arg options are still possible? If so, how does the API distinguish between them?

For example, a --plugins option should still be able to accept multiple values. (e.g. "--plugins @robojs/ai @robojs/sync")

Rishi-0007 · 2024-10-27T10:06:27Z

Thank you for this! Can you please verify that multi-arg options are still possible? If so, how does the API distinguish between them?

For example, a --plugins option should still be able to accept multiple values. (e.g. "--plugins @robojs/ai @robojs/sync")

Hey @Pkmmte , I've made the changes based on your feedback. Please check it and let me know if this approach is good or any further changes are required.

Pkmmte

Hi, we've marked this submission as accepted for Hacktoberfest purposes because there's some improvements in here we like, but overall we've found some regressions.

For example, positional arguments no longer work.

npx robo build src/commands/ping.ts -v

For some reason, it treats options (-v) as positional arguments themselves.

info  - Building Robo...
node:internal/fs/promises:1037
  const result = await PromisePrototypeThen(
                 ^

Error: ENOENT: no such file or directory, stat '/Users/pkmmte/demo/cli-update-1/-v'
    at async Object.stat (node:internal/fs/promises:1037:18)
    at async traverse (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/compiler.js:59:18)
    at async Object.buildCode (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/compiler.js:133:3)
    at async Command.buildAction [as _handler] (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/commands/build/index.js:63:23)
    at async Command.processSubCommand (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/cli-handler.js:255:5)
    at async Command.processSubCommand (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/cli-handler.js:245:9) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: '/Users/pkmmte/demo/cli-update-1/-v'
}

Node.js v22.2.0

(ENOENT in this case because build looks for positional arguments in the file system)

Changes expected behavior

The type signature and behavior for multiple options (enabled by default) is also changed. Previously, options with spaces would still yield a single string, but it looks like this now changes to a string array.

While I agree that an array would be more appropriate, we like to avoid changing behavior like this unless it's a major version release. It is likely to break existing code reliant on options always being a single string. In fact, our own create-robo package would break from this change because of how it uses --plugins.

As stated earlier, we will be accepting this PR for Hacktoberfest purposes, but may delay in merging due to the above issues.

Rishi-0007 · 2024-10-28T20:30:33Z

Hi @Pkmmte,

Thank you for your feedback and for accepting the PR for Hacktoberfest. I’ve been working on addressing the issues mentioned, specifically ensuring options like -v after positional arguments are correctly parsed and not treated as file paths.

Here’s what I’ve tried so far:

Adjusted parseOptions in cli-handler.ts: Refined the parsing logic to separate options from positional arguments and ensure options with spaces return single strings by default.
Verified Argument Parsing with Tests: Added and confirmed tests for scenarios with options appearing after positional arguments, ensuring options are recognized correctly.
Encountering Persistent ENOENT Error: When running the command:
```
npx robo build src/cli/commands/add.ts -v
```
the -v flag continues to be interpreted as a positional argument, causing an attempt to locate a file at -v, and resulting in the ENOENT error. This behavior appears on both the main branch and my modified branch, suggesting it may be due to the overall handling of positional arguments in the build command.

Would you be able to provide additional guidance on addressing this issue? Specifically, are there any expected parsing behaviors or modifications within cli-handler.ts that would prevent options like -v from being treated as positional arguments in the context of the build command?

Thank you for your time and assistance.

Nazeofel · 2024-10-28T21:11:54Z

Hello ! thanks for your contribution, I have just tested it out, and it seems to be working just fine for me, are you sure to be testing out the right branch ?

Rishi-0007 · 2024-10-29T06:13:32Z

Hello ! thanks for your contribution, I have just tested it out, and it seems to be working just fine for me, are you sure to be testing out the right branch ?

Hi @Nazeofel, Thanks for testing it out.
Is this the expected output or am I doing something wrong?

npx robo build src/cli/commands/add.ts -v
info  - Building Robo...
node:internal/fs/promises:1032
  const result = await PromisePrototypeThen(
                 ^

Error: ENOENT: no such file or directory, stat '/home/rishi/code/robo.js/packages/robo/-v'
    at async Object.stat (node:internal/fs/promises:1032:18)
    at async traverse (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils/compiler.js:59:1
8)                                                                                                      at async Object.buildCode (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils/compiler
.js:133:3)                                                                                              at async Command.buildAction [as _handler] (file:///home/rishi/code/robo.js/packages/robo/dist/c
li/commands/build/index.js:63:23)                                                                       at async Command.processSubCommand (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils
/cli-handler.js:255:5)                                                                                  at async Command.processSubCommand (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils
/cli-handler.js:245:9) {                                                                              errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: '/home/rishi/code/robo.js/packages/robo/-v'
}

Node.js v22.9.0

But when I removed -v, I got this output(which is not an error I guess):

npx robo build src/cli/commands/add.ts   
info  - Building Robo...

Type                Name            Description
───────────────────────────────────────────────
Command            Δ /help            Displays a list of commands.
Event              Δ interactionCreate
Event              Δ ready  

Δ = Automatically generated

Robo size: 836.92 kB
Built in 113ms

discord:error - DISCORD_TOKEN or DISCORD_CLIENT_ID not found in environment variables

Is this what you are talking about in the previous comment @Pkmmte?

Rishi-0007 · 2024-10-29T06:39:45Z

Hey @Pkmmte,

I’ve tested the issue on the main branch as well and observed the following outputs in two cases:

Without -v:

npx robo build src/cli/commands/add.ts
info  - Building Robo...

Type                Name            Description
───────────────────────────────────────────────
Command            Δ /help            Displays a list of commands.
Event              Δ interactionCreate
Event              Δ ready  

Δ = Automatically generated

Robo size: 836.59 kB
Built in 109ms

discord:error - DISCORD_TOKEN or DISCORD_CLIENT_ID not found in environment variables

With -v:

npx robo build src/cli/commands/add.ts -v
info  - Building Robo...
node:internal/fs/promises:1032
  const result = await PromisePrototypeThen(
                 ^

Error: ENOENT: no such file or directory, stat '/home/rishi/code/robo.js/packages/robo/-v'
    at async Object.stat (node:internal/fs/promises:1032:18)
    at async traverse (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils/compiler.js:59:1
8)                                                                                                      at async Object.buildCode (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils/compiler
.js:133:3)                                                                                              at async Command.buildAction [as _handler] (file:///home/rishi/code/robo.js/packages/robo/dist/c
li/commands/build/index.js:63:23)                                                                       at async Command.processSubCommand (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils
/cli-handler.js:255:5)                                                                                  at async Command.processSubCommand (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils
/cli-handler.js:245:9) {                                                                              errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: '/home/rishi/code/robo.js/packages/robo/-v'
}

Node.js v22.9.0

This ENOENT error seems unrelated to my changes since it’s happening on the main branch as well. Could you confirm if I’m missing something or if there’s another approach I should take? Your guidance would be greatly appreciated.

Thank you!

Nazeofel · 2024-10-29T16:48:30Z

Hey ! it should work just fine on the main branch and latest version of Robo, just to confirm somethin, which OS are you running ?

Rishi-0007 · 2024-10-29T18:04:38Z

Hey ! it should work just fine on the main branch and latest version of Robo, just to confirm somethin, which OS are you running ?

Ubuntu noble 24.04 x86_64

Nazeofel · 2024-10-30T19:19:07Z

Hey sorry to get back to you so late, we cannot reproduce this issue at all on our environment with the main branch, tested on MacOS, Windows, and Linux too. would you be able to share your all Linux configuration where you are running the command please.

fix(cli-handler): make multi-arg options optional, capturing only sin…

0da3443

…gle values Ensure options like capture only one argument, leaving positional arguments intact.

vercel bot deployed to Preview October 26, 2024 13:00 View deployment

Pkmmte added a commit that referenced this pull request Oct 26, 2024

@Rishi-0007 has signed the CLA in #334

ed92c32

fix(cli-handler): handle multiple values for options with separator

5a74ab3

vercel bot deployed to Preview October 27, 2024 10:03 View deployment

Pkmmte added the hacktoberfest label Oct 27, 2024

Pkmmte self-assigned this Oct 27, 2024

Pkmmte self-requested a review October 27, 2024 18:49

Added comments.

9082b5d

vercel bot deployed to Preview October 27, 2024 19:21 View deployment

Pkmmte added the hacktoberfest-accepted label Oct 27, 2024

Pkmmte requested changes Oct 27, 2024

View reviewed changes

refactor(cli-handler): improve option parsing logic

2cd9901

vercel bot deployed to Preview October 28, 2024 06:44 View deployment

Rishi-0007 requested a review from Pkmmte October 28, 2024 19:27

refactor: cli-handler & test

38da7ba

vercel bot deployed to Preview October 28, 2024 20:22 View deployment

Added comments in cli-handle.ts

7bac800

vercel bot deployed to Preview October 28, 2024 20:37 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make multi-arg options optional #334

Make multi-arg options optional #334

Rishi-0007 commented Oct 26, 2024 •

edited

Loading

vercel bot commented Oct 26, 2024 •

edited

Loading

Pkmmte commented Oct 26, 2024 •

edited

Loading

Rishi-0007 commented Oct 26, 2024

Pkmmte commented Oct 26, 2024

Rishi-0007 commented Oct 27, 2024

Pkmmte left a comment

Rishi-0007 commented Oct 28, 2024

Nazeofel commented Oct 28, 2024

Rishi-0007 commented Oct 29, 2024 •

edited

Loading

Rishi-0007 commented Oct 29, 2024

Nazeofel commented Oct 29, 2024

Rishi-0007 commented Oct 29, 2024

Nazeofel commented Oct 30, 2024

Make multi-arg options optional #334

Are you sure you want to change the base?

Make multi-arg options optional #334

Conversation

Rishi-0007 commented Oct 26, 2024 • edited Loading

vercel bot commented Oct 26, 2024 • edited Loading

Pkmmte commented Oct 26, 2024 • edited Loading

Rishi-0007 commented Oct 26, 2024

Pkmmte commented Oct 26, 2024

Rishi-0007 commented Oct 27, 2024

Pkmmte left a comment

Choose a reason for hiding this comment

For example, positional arguments no longer work.

Changes expected behavior

Rishi-0007 commented Oct 28, 2024

Nazeofel commented Oct 28, 2024

Rishi-0007 commented Oct 29, 2024 • edited Loading

Rishi-0007 commented Oct 29, 2024

Nazeofel commented Oct 29, 2024

Rishi-0007 commented Oct 29, 2024

Nazeofel commented Oct 30, 2024

Rishi-0007 commented Oct 26, 2024 •

edited

Loading

vercel bot commented Oct 26, 2024 •

edited

Loading

Pkmmte commented Oct 26, 2024 •

edited

Loading

Rishi-0007 commented Oct 29, 2024 •

edited

Loading